Skip to content

Fix missing updated time on migrated issues and comments #9744

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 3 commits into from
Jan 14, 2020
Merged

Fix missing updated time on migrated issues and comments #9744

merged 3 commits into from
Jan 14, 2020

Conversation

lunny
Copy link
Member

@lunny lunny commented Jan 13, 2020

Should fix #9741

@lunny lunny added type/bug skip-changelog This PR is irrelevant for the (next) changelog, for example bug fixes for unreleased features. labels Jan 13, 2020
@lunny lunny modified the milestones: 1.11.0, 1.12.0 Jan 13, 2020
@GiteaBot GiteaBot added the lgtm/need 1 This PR needs approval from one additional maintainer to be merged. label Jan 13, 2020
sapk
sapk previously approved these changes Jan 13, 2020
View reviewed changes
Copy link
Member

@sapk sapk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need adjustement on test data otherwise LGTM

@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Jan 13, 2020
@6543
Copy link
Member

6543 commented Jan 13, 2020

@lunny found second bug:
func (g *GithubDownloaderV3) GetPullRequests(page, perPage int) ([]*base.PullRequest, error) {
need a patch too around Line 525 link

6543
6543 requested changes Jan 13, 2020
View reviewed changes
Copy link
Member

@6543 6543 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as comment

@6543
Copy link
Member

6543 commented Jan 13, 2020

@sapk no there is a missing line ...

@6543

This comment has been minimized.

Sign in to view
@sapk sapk dismissed their stale review January 13, 2020 16:00

seems to need more fix

@GiteaBot GiteaBot added the lgtm/need 1 This PR needs approval from one additional maintainer to be merged. label Jan 13, 2020
@sapk sapk removed the lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. label Jan 13, 2020
@lunny lunny force-pushed the lunny/fix_migrate_comment branch from b9eefdf to b00a6bc Compare January 14, 2020 00:04
@codecov-io
Copy link

Codecov Report

Merging #9744 into master will increase coverage by <.01%.
The diff coverage is 50%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #9744      +/-   ##
=========================================
+ Coverage    42.3%   42.3%   +<.01%     
=========================================
  Files         598     598              
  Lines       78318   78324       +6     
=========================================
+ Hits        33135   33138       +3     
- Misses      41127   41130       +3     
  Partials     4056    4056
Impacted Files Coverage Δ
modules/migrations/base/pullrequest.go 0% <ø> (ø) ⬆️
modules/migrations/gitea.go 7.91% <0%> (-0.05%) ⬇️
modules/migrations/github.go 77.46% <100%> (+0.15%) ⬆️
services/pull/check.go 48.64% <0%> (-4.73%) ⬇️
modules/log/event.go 64.61% <0%> (-1.03%) ⬇️
models/pull.go 42.47% <0%> (+0.6%) ⬆️
models/webhook.go 70.46% <0%> (+1.06%) ⬆️
modules/queue/workerpool.go 41.2% <0%> (+1.28%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1751d5f...b00a6bc. Read the comment docs.

@techknowlogick
Copy link
Member

Is a new migration needed for these changes (not possible for backport, but what about master)?

@6543
Copy link
Member

6543 commented Jan 14, 2020

migration is only needed if we like to fix old records

@techknowlogick
Copy link
Member

@6543 but this is new column

@lunny
Copy link
Member Author

lunny commented Jan 14, 2020

@techknowlogick will add a migration.

@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Jan 14, 2020
@6543
Copy link
Member

6543 commented Jan 14, 2020

@techknowlogick colum already exist in db. But with wrong entrys for migrated issues ...

@techknowlogick
Copy link
Member

@6543 ah ok. Thanks for pointing that out. I thought this was a different struct.

@zeripath
Copy link
Contributor

Make lg-tm work

@zeripath zeripath merged commit 7f869c0 into go-gitea:master Jan 14, 2020
@lunny lunny deleted the lunny/fix_migrate_comment branch January 14, 2020 16:21
lunny added a commit to lunny/gitea that referenced this pull request Jan 14, 2020
@lunny @sapk
Fix missing updated time on migrated issues and comments (go-gitea#9744)
f55da38 
* Fix missing updated time on migrated issues and comments

* Fix testing and missing updated on migrating pullrequest

Co-authored-by: Antoine GIRARD <[email protected]>
@lunny lunny mentioned this pull request Jan 14, 2020
Merged
@lunny lunny added the backport/done All backports for this PR have been created label Jan 14, 2020
sapk added a commit that referenced this pull request Jan 14, 2020
@lunny @sapk
Fix missing updated time on migrated issues and comments (#9744) (#9764)
2850879 
* Fix missing updated time on migrated issues and comments

* Fix testing and missing updated on migrating pullrequest

Co-authored-by: Antoine GIRARD <[email protected]>

Co-authored-by: Antoine GIRARD <[email protected]>
@go-gitea go-gitea locked and limited conversation to collaborators Nov 24, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@6543 6543 6543 approved these changes

+2 more reviewers

@guillep2k guillep2k guillep2k approved these changes

@sapk sapk sapk left review comments

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
backport/done All backports for this PR have been created lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. skip-changelog This PR is irrelevant for the (next) changelog, for example bug fixes for unreleased features. type/bug
Projects
None yet
Milestone
1.12.0
Development

Successfully merging this pull request may close these issues.

Wrong meta info at issueComments on github migration
8 participants
@lunny @6543 @codecov-io @techknowlogick @zeripath @sapk @guillep2k @GiteaBot